Skip to content

[SPARK-20214][ML] Make sure converted csc matrix has sorted indices#17532

Closed
viirya wants to merge 2 commits intoapache:masterfrom
viirya:make-sure-sorted-indices
Closed

[SPARK-20214][ML] Make sure converted csc matrix has sorted indices#17532
viirya wants to merge 2 commits intoapache:masterfrom
viirya:make-sure-sorted-indices

Conversation

@viirya
Copy link
Copy Markdown
Member

@viirya viirya commented Apr 5, 2017

What changes were proposed in this pull request?

_convert_to_vector converts a scipy sparse matrix to csc matrix for initializing SparseVector. However, it doesn't guarantee the converted csc matrix has sorted indices and so a failure happens when you do something like that:

from scipy.sparse import lil_matrix
lil = lil_matrix((4, 1))
lil[1, 0] = 1
lil[3, 0] = 2
_convert_to_vector(lil.todok())

File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 78, in _convert_to_vector
  return SparseVector(l.shape[0], csc.indices, csc.data)
File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 556, in __init__
  % (self.indices[i], self.indices[i + 1]))
TypeError: Indices 3 and 1 are not strictly increasing

A simple test can confirm that dok_matrix.tocsc() won't guarantee sorted indices:

>>> from scipy.sparse import lil_matrix
>>> lil = lil_matrix((4, 1))
>>> lil[1, 0] = 1
>>> lil[3, 0] = 2
>>> dok = lil.todok()
>>> csc = dok.tocsc()
>>> csc.has_sorted_indices
0
>>> csc.indices
array([3, 1], dtype=int32)

I checked the source codes of scipy. The only way to guarantee it is csc_matrix.tocsr() and csr_matrix.tocsc().

How was this patch tested?

Existing tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@viirya viirya force-pushed the make-sure-sorted-indices branch from f88eb6b to 10be94a Compare April 5, 2017 03:32
@viirya
Copy link
Copy Markdown
Member Author

viirya commented Apr 5, 2017

cc @jkbradley @MLnick @holdenk

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 5, 2017

Test build #75528 has finished for PR 17532 at commit f88eb6b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 5, 2017

Test build #75530 has finished for PR 17532 at commit 10be94a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Copy Markdown
Member

srowen commented Apr 5, 2017

I don't think is directly related, but, I tried to make a similar change to the Spark Scala implementation of this type of conversion and couldn't make it work, just because it changed a bunch of little behaviors. (I could look up the JIRA.) This could be fine, being more narrow.

jkbradley
jkbradley reviewed Apr 5, 2017
View reviewed changes
Comment thread python/pyspark/ml/linalg/__init__.py Outdated
assert l.shape[1] == 1, "Expected column vector"
csc = l.tocsc()
# Make sure the converted csc_matrix has sorted indices.
csc = l.tocsr().tocsc()
Copy link
Copy Markdown
Member

@jkbradley jkbradley Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just call tocsc. You can then check the attribute has_sorted_indices and call sort_indices() if needed.

Copy link
Copy Markdown
Member Author

@viirya viirya Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed.

@jkbradley
Copy link
Copy Markdown
Member

jkbradley commented Apr 5, 2017

Are you able to write a unit test which passes data through _convert_to_vector and fails before this fix?

@jkbradley
Copy link
Copy Markdown
Member

jkbradley commented Apr 5, 2017

Btw, I'd really like to get this into 2.2, which will be cut soon. Let me know if you'd like me to take it over. Thanks!

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Apr 5, 2017

@jkbradley Thanks for comment. I will add the unit test now.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Apr 5, 2017

@jkbradley An unit test is added. Please check this again. Thanks.

viirya
viirya commented Apr 5, 2017
View reviewed changes
Comment thread python/pyspark/mllib/tests.py
self.assertEqual(sv, serialize(lil.tocsr()))
self.assertEqual(sv, serialize(lil.todok()))

def test_convert_to_vector(self):
Copy link
Copy Markdown
Member Author

@viirya viirya Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no Scipy-related tests in ml submodule, so I don't add it. If we really need one, please let me know. I can add it quickly.

Copy link
Copy Markdown
Member

@jkbradley jkbradley Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's Ok; I made a separate JIRA to port the tests there.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 6, 2017

Test build #75560 has finished for PR 17532 at commit 2612d66.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Copy Markdown
Member

jkbradley commented Apr 6, 2017

LGTM
Merging with master and branch-2.1, branch-2.0
Thanks a lot!

asfgit pushed a commit that referenced this pull request Apr 6, 2017
@viirya @jkbradley
[SPARK-20214][ML] Make sure converted csc matrix has sorted indices
fb81a41 
## What changes were proposed in this pull request?

`_convert_to_vector` converts a scipy sparse matrix to csc matrix for initializing `SparseVector`. However, it doesn't guarantee the converted csc matrix has sorted indices and so a failure happens when you do something like that:

    from scipy.sparse import lil_matrix
    lil = lil_matrix((4, 1))
    lil[1, 0] = 1
    lil[3, 0] = 2
    _convert_to_vector(lil.todok())

    File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 78, in _convert_to_vector
      return SparseVector(l.shape[0], csc.indices, csc.data)
    File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 556, in __init__
      % (self.indices[i], self.indices[i + 1]))
    TypeError: Indices 3 and 1 are not strictly increasing

A simple test can confirm that `dok_matrix.tocsc()` won't guarantee sorted indices:

    >>> from scipy.sparse import lil_matrix
    >>> lil = lil_matrix((4, 1))
    >>> lil[1, 0] = 1
    >>> lil[3, 0] = 2
    >>> dok = lil.todok()
    >>> csc = dok.tocsc()
    >>> csc.has_sorted_indices
    0
    >>> csc.indices
    array([3, 1], dtype=int32)

I checked the source codes of scipy. The only way to guarantee it is `csc_matrix.tocsr()` and `csr_matrix.tocsc()`.

## How was this patch tested?

Existing tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes #17532 from viirya/make-sure-sorted-indices.

(cherry picked from commit 1220605)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
@asfgit asfgit closed this in 1220605 Apr 6, 2017
asfgit pushed a commit that referenced this pull request Apr 6, 2017
@viirya @jkbradley
[SPARK-20214][ML] Make sure converted csc matrix has sorted indices
9016e17 
## What changes were proposed in this pull request?

`_convert_to_vector` converts a scipy sparse matrix to csc matrix for initializing `SparseVector`. However, it doesn't guarantee the converted csc matrix has sorted indices and so a failure happens when you do something like that:

    from scipy.sparse import lil_matrix
    lil = lil_matrix((4, 1))
    lil[1, 0] = 1
    lil[3, 0] = 2
    _convert_to_vector(lil.todok())

    File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 78, in _convert_to_vector
      return SparseVector(l.shape[0], csc.indices, csc.data)
    File "/home/jenkins/workspace/python/pyspark/mllib/linalg/__init__.py", line 556, in __init__
      % (self.indices[i], self.indices[i + 1]))
    TypeError: Indices 3 and 1 are not strictly increasing

A simple test can confirm that `dok_matrix.tocsc()` won't guarantee sorted indices:

    >>> from scipy.sparse import lil_matrix
    >>> lil = lil_matrix((4, 1))
    >>> lil[1, 0] = 1
    >>> lil[3, 0] = 2
    >>> dok = lil.todok()
    >>> csc = dok.tocsc()
    >>> csc.has_sorted_indices
    0
    >>> csc.indices
    array([3, 1], dtype=int32)

I checked the source codes of scipy. The only way to guarantee it is `csc_matrix.tocsr()` and `csr_matrix.tocsc()`.

## How was this patch tested?

Existing tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes #17532 from viirya/make-sure-sorted-indices.

(cherry picked from commit 1220605)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
@viirya viirya deleted the make-sure-sorted-indices branch December 27, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jkbradley jkbradley jkbradley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya @SparkQA @srowen @jkbradley